🦋 Changeset detected

Latest commit: d70a839

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Code Review

One bug was fixed directly (commit 5965f69). Three issues below require attention.

Fixed: #each key used : as delimiter — inconsistent with serializeErrors and collision-prone

queries.svelte (original line 64)

{#each errors as error (`${error.name}:${error.message}`)}

Using : as the separator means any error whose .name contains a colon (e.g. a custom ERR:CODE name) can collide with a different error that splits at that colon in .message. serializeErrors already uses \0 precisely to avoid this. Fixed in the follow-up commit to align both usages.

Issue: setTimeout not cancelled on component teardown

src/lib/components/error.svelte, lines 25–27

setTimeout(() => {
    showCopySuccess = false
}, 750)

The timeout ID is never stored or cleared. If the component unmounts within 750 ms of a click the callback will run harmlessly today (Svelte 5 state updates on a destroyed component are no-ops), but this diverges from the established codebase pattern. refetch-controller.svelte (lines 87–90) shows the correct approach:

let timeoutId: ReturnType<typeof setTimeout> | undefined

const copyErrorToClipboard = async () => {
    try {
        await globalThis.navigator.clipboard.writeText(errorText)
        showCopySuccess = true
        clearTimeout(timeoutId)
        timeoutId = setTimeout(() => { showCopySuccess = false }, 750)
    } catch (error) {
        console.error('Failed to copy error to clipboard', error)
    }
}

$effect(() => () => clearTimeout(timeoutId))

The clearTimeout before re-assigning also prevents stacking multiple timers if the user clicks rapidly.

Issue: No unit tests for the serializeErrors guard in queries.svelte

The new serializeErrors comparison guard (queries.svelte lines 40–42) has zero test coverage. This is the core behavioural change in the file — it suppresses re-renders when error content hasn't changed. Missing edge cases:

• Guard passes (different errors → state updates).
• Guard blocks (same errors → no state update, selection preserved).
• Guard resets correctly when all queries succeed (errors = []).
• Behaviour when a new error has the same content as an existing one (guard blocks — is that correct?).

Please add a queries.spec.ts similar to refetch-controller.spec.ts.

@claude resolve these

Claude finished @DTCurrie's task in 4m 21s —— View job

Resolved Review Issues

• Read CLAUDE.md and gather context
• Fix setTimeout not cancelled on component teardown in error.svelte
• Add unit tests for serializeErrors guard in queries.svelte
• Run lint, type-check, and tests
• Commit and push

Changes made (commit 37a3d33)

src/lib/components/error.svelte — timeout teardown fix

Added copySuccessTimeoutId tracking, clearTimeout before re-assigning (prevents timer stacking on rapid clicks), and a $effect cleanup — matching the pattern in refetch-controller.svelte:

let copySuccessTimeoutId: ReturnType<typeof setTimeout> | undefined

const copyErrorToClipboard = async () => {
    try {
        await globalThis.navigator.clipboard.writeText(errorText)
        showCopySuccess = true
        clearTimeout(copySuccessTimeoutId)
        copySuccessTimeoutId = setTimeout(() => { showCopySuccess = false }, 750)
    } catch (error) {
        console.error('Failed to copy error to clipboard', error)
    }
}

$effect(() => () => clearTimeout(copySuccessTimeoutId))

src/lib/components/__tests__/queries.spec.ts — new file, 5 tests covering the serializeErrors guard:

Note: pnpm lint has a pre-existing Prettier warning on .ci-prompts/prompts/ci-common.md (untracked file, not part of this PR). pnpm check passes with 0 errors/warnings. Tests require Playwright which isn't installed on this runner — CI will run the full suite.
| Branch

@claude re-review

Claude finished @DTCurrie's task in 3m 30s —— View job

Re-review

• Read CLAUDE.md and gather context
• Read all changed files (error.svelte, queries.svelte, error.spec.ts, queries.spec.ts, changeset)
• Run pnpm check and pnpm lint
• Review code and post feedback

Summary

All three issues from the previous review are resolved. pnpm check passes with 0 errors/0 warnings. The lint failure is the pre-existing Prettier warning on .ci-prompts/prompts/ci-common.md (untracked, not part of this PR).

Resolved ✅

#each key delimiter — now uses \0 in both the #each key (queries.svelte:64) and serializeErrors, fully consistent.

setTimeout teardown — copySuccessTimeoutId is stored, clearTimeout is called before re-assigning (prevents timer stacking), and $effect(() => () => clearTimeout(...)) cleans up on unmount. Matches the pattern in refetch-controller.svelte. Consistent with readings-list.svelte which uses the same Tooltip/IconButton/on:click API from prime-core.

serializeErrors guard tests — queries.spec.ts covers all four originally-requested edge cases (guard passes, guard blocks, resets on success, preserves during loading) plus an explicit duplicate-content test.

Observations (non-blocking)

serializeErrors separator — see inline comment on queries.svelte:33. Theoretical \n+\0 collision is extremely unlikely in practice but easy to harden. Not a blocker.

error.spec.ts — missing icon-swap test — see inline comment on error.spec.ts:51. The showCopySuccess visual feedback path (icon changes to check) isn't tested. Worth adding to lock in the UX behaviour.

Screenshot UI artifact — the screenshot from @DTCurrie shows "Copy error" appearing both as a tooltip overlay and inline within the widget description text. This is likely the Tooltip rendering its portal content at the wrong position when the button sits inside a flex row that is itself inside a widget card. Worth a manual pass in different widget contexts to confirm the tooltip positions correctly before merging.

queries.spec.ts near-duplicate tests (lines 55-65 and 86-97) — both test that identical content blocks state update. The second test's comment makes the intent clear (explicit documentation that duplicate-content deduplication is intentional), so the duplication is acceptable.

The three originally-raised issues are fully fixed. The two minor gaps (icon-swap test, separator edge case) are worth addressing but not blocking. The tooltip positioning question from the screenshot is the main thing I'd verify manually before merge.
| Branch

@claude resolve these:

serializeErrors separator — see inline comment on queries.svelte:33. Theoretical \n+\0 collision is extremely unlikely in practice but easy to harden. Not a blocker.

error.spec.ts — missing icon-swap test — see inline comment on error.spec.ts:51. The showCopySuccess visual feedback path (icon changes to check) isn't tested. Worth adding to lock in the UX behaviour.

Claude finished @DTCurrie's task in 2m 29s —— View job

Resolved Review Issues

• Read CLAUDE.md and gather context
• Harden serializeErrors separator in queries.svelte:33
• Add icon-swap test to error.spec.ts
• Run pnpm check (0 errors, 0 warnings) — lint only fails on pre-existing .ci-prompts/ issue unrelated to this PR
• Commit and push (commit b4c6688)

Changes made

src/lib/components/queries.svelte:33 — separator hardened

- errs.map((error) => `${error.name}\0${error.message}`).join('\n')
+ errs.map((error) => `${error.name}\0${error.message}`).join('\0\0')

Using \0\0 as the array separator makes serializeErrors collision-proof: a message containing \n\0 could previously produce a false match, but no error message can contain \0\0 without also being a valid array boundary.

src/lib/components/__tests__/error.spec.ts — icon-swap test added

New test 'shows check icon after successful copy and restores content-copy icon after timeout':

1. Verifies icon-content-copy is present before click (via getByTestId)
2. Clicks copy button, asserts icon-check appears (using waitFor since the handler is async)
3. Runs fake timers past the 750 ms timeout, asserts icon-content-copy restores and icon-check is gone

Mirrors the getByTestId('icon-refresh') pattern already used in refetch-controller.spec.ts.
| Branch

@claude it looks like your test failed: https://github.com/viamrobotics/test-widgets/actions/runs/25187465783/job/73848431474?pr=38

Claude finished @DTCurrie's task in 4m 0s —— View job

Fixed the failing test (commit d70a839).

• Read CLAUDE.md and gather context
• Download and analyze CI failure logs
• Diagnose the test failure
• Fix the issue
• Run verification (pnpm check — 0 errors/warnings)
• Commit and push

Root cause

user = userEvent.setup() was called in beforeEach with real timers. The icon-swap test then called vi.useFakeTimers() mid-test. @testing-library/user-event v14 uses setTimeout(fn, 0) internally to sequence pointer events; once fake timers replaced setTimeout, await user.click(...) hung waiting for a 0 ms fake timeout that never fired — causing the 15 s test timeout.

Fix (error.spec.ts:52)

Created a local userEvent instance inside the failing test with the advanceTimers option, so it drives fake timer advancement itself:

-   await user.click(copyButton)
+   const timerUser = userEvent.setup({ advanceTimers: vi.advanceTimersByTime.bind(vi) })
+   // ...
+   await timerUser.click(copyButton)

The outer user (set up in beforeEach) is still used by all other tests — they don't use fake timers and are unaffected.
| Branch